Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: add support for template steps/configs in reports #246

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Jul 3, 2024

Description

Adds report output for template steps/reports

More refining to do, but the start is here

Motivation and Context

Reports are basically the reason atef exists.

We probably want some better reprs for the RegexFindReplace classes.

How Has This Been Tested?

interactively

Where Has This Been Documented?

This PR

image

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Code has been checked for threading issues (no blocking tasks in GUI thread)
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong tangkong marked this pull request as ready for review July 10, 2024 18:09
@tangkong tangkong requested a review from ZLLentz July 10, 2024 18:09
@ZLLentz
Copy link
Member

ZLLentz commented Jul 10, 2024

  • Looks and works good for me 👍
  • Didn't see anything abnormal in the implementation
  • Note that there still isn't anything in the in-UI summary table but I appreciate that the UI is always another can of worms
  • Rendering of the edits in the pdf is a bit hard to understand when there is more than one edit but it's not so critical.
  • Visually it doesn't match what's in the UI but it does match what's in the file (somehow, my file ended up with 3x copies of the search regex? how did that happen?)

image
image

/cds/home/z/zlentz/github/atef/imager_templates.json

ZLLentz
ZLLentz previously approved these changes Jul 10, 2024
@tangkong
Copy link
Contributor Author

Strangely those search regexs all have different paths, so that part was correct. They do not represent the search and replace text properly though.

Could you remind me exactly which in-UI table you're looking at, where template checkout support is missing?

@ZLLentz
Copy link
Member

ZLLentz commented Jul 10, 2024

I re-read the json and the results and I see where I got confused before (my brain didn't even see the differences between the three elements)

This is the in-ui table:
image

@tangkong
Copy link
Contributor Author

aah gotcha. Yea that is something to consider

@tangkong
Copy link
Contributor Author

whoops forgot about this. Merging

@tangkong
Copy link
Contributor Author

tangkong commented Aug 6, 2024

Whoops I forgot about this entirely, and had to do some merge conflict fixing. Requesting another review, nothing of substance has changed

@tangkong tangkong requested a review from ZLLentz August 6, 2024 16:51
@ZLLentz
Copy link
Member

ZLLentz commented Aug 7, 2024

The test suite failures are somewhat concerning in that I don't really understand them but they aren't new. The code is the same. If you re-opened the gui yourself after this I'm 👍 on merging.

@tangkong
Copy link
Contributor Author

tangkong commented Aug 7, 2024

i have re-opened the GUI... 😞 dumb conda. I'll just merge and hope this resolves itself

@tangkong tangkong merged commit ab04444 into pcdshub:master Aug 7, 2024
7 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants